feat(agent,agent-installer): add transactional policy store - #1963
feat(agent,agent-installer): add transactional policy store#1963Benoît Cortier (CBenoit) wants to merge 31 commits into
Conversation
|
Implementation notes:
Note Human-tuned, LLM-assisted content. |
|
CI repair update (
Focused validation passed: 37 policy-store Windows tests, 3 token-wrapper tests, agent-policy-tester check, direct unelevated E2E with token assertion, rustfmt, diff check, and targeted Clippy with warnings denied. Workflow assertions for restricted launch and single message-compiler selection also passed; Note Human-tuned, LLM-assisted content. |
|
Follow-up CI repair ( Local script parsing, workflow assertions, diff/rustfmt, agent-policy-tester check, and targeted Clippy passed. Note Human-tuned, LLM-assisted content. |
|
Final CI status: run 33254105745 passed at Note Human-tuned, LLM-assisted content. |
|
Shared-contract refresh provenance for
All six local artifacts were rehashed and matched Local validation: rustfmt and diff checks passed; locked broker suites passed 409/413 tests respectively with only the documented host Winget AppExecAlias DACL baseline; locked targeted check and Clippy with warnings denied passed; Agent and tester builds passed; unelevated Agent policy E2E passed. Note Human-tuned, LLM-assisted content. |
|
Authoritative shared-contract refresh at
All six unpublished artifacts were rehashed and match Local validation: rustfmt and diff checks passed; locked broker suites passed 409/413 tests respectively with only the documented host Winget AppExecAlias DACL baseline; locked targeted check and Clippy with warnings denied passed; Agent and tester builds passed; unelevated Agent policy E2E passed. Exact-head CI: 33764438462. Note Human-tuned, LLM-assisted content. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved upgrade, filesystem-concurrency, authorization, and installer-security issues undermine the transactional guarantees.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds authenticated transactional JSON policy management to Devolutions Agent, including secure persistence, validation, auditing, and end-to-end coverage.
Changes:
- Replaces the legacy loader/watcher with a transactional policy store.
- Adds elevation, ACL, concurrency-token, and Windows Event Log protections.
- Expands Windows CI and privileged/unelevated testing.
File summaries
| File | Description |
|---|---|
.github/workflows/ci.yml |
Adds SDK discovery and policy test modes. |
Cargo.lock |
Locks new contract and security dependencies. |
Cargo.toml |
Pins Phase 2 policy crates. |
crates/agent-policy-tester/Cargo.toml |
Adds Windows policy-test dependencies. |
crates/agent-policy-tester/run-as-system.ps1 |
Selects elevated test mode. |
crates/agent-policy-tester/run-unelevated.ps1 |
Adds restricted-token test runner. |
crates/agent-policy-tester/src/windows.rs |
Tests management lifecycle and authorization. |
crates/now-package-broker/Cargo.toml |
Adds receipts and audit dependencies. |
crates/now-package-broker/src/audit.rs |
Implements structured policy auditing. |
crates/now-package-broker/src/auth.rs |
Captures elevation and administrator membership. |
crates/now-package-broker/src/evaluator/mod.rs |
Exposes wildcard validation internally. |
crates/now-package-broker/src/evaluator/wildcard.rs |
Shares wildcard compilation checks. |
crates/now-package-broker/src/lib.rs |
Registers policy-store modules. |
crates/now-package-broker/src/policy_loader.rs |
Removes the legacy loader. |
crates/now-package-broker/src/policy_security.rs |
Strengthens policy filesystem security. |
crates/now-package-broker/src/policy_store/mod.rs |
Implements transactional store coordination. |
crates/now-package-broker/src/policy_store/receipt.rs |
Adds keyed validation receipts. |
crates/now-package-broker/src/policy_store/validation.rs |
Implements deterministic policy validation. |
crates/now-package-broker/src/policy_store/windows.rs |
Implements secure Windows persistence. |
crates/now-package-broker/src/policy_watcher.rs |
Removes the legacy watcher. |
crates/now-package-broker/src/scenario_tests.rs |
Makes scenarios JSON-only. |
crates/now-package-broker/src/server/mod.rs |
Adds policy-management routes and authorization. |
crates/now-package-broker/src/server/responses.rs |
Maps management errors and snapshots. |
crates/now-package-broker/src/task.rs |
Integrates the transactional store. |
crates/now-package-broker/src/test_support.rs |
Adds shared SID test support. |
crates/sysevent-codes/src/lib.rs |
Defines policy audit events. |
crates/sysevent-codes/tests/message_catalog_parity.rs |
Checks event-catalog parity. |
crates/sysevent-winevent/src/lib.rs |
Corrects null-handle detection. |
crates/win-api-wrappers/src/token.rs |
Adds token group-membership checks. |
devolutions-agent/build.rs |
Embeds the Agent message catalog. |
devolutions-agent/devolutions-agent.mc |
Adds the Agent event catalog. |
devolutions-gateway/devolutions-gateway.mc |
Mirrors policy audit messages. |
package/AgentWindowsManaged/Actions/AgentActions.cs |
Schedules policy-directory actions. |
package/AgentWindowsManaged/Actions/CustomActions.cs |
Creates and secures the policy directory. |
package/AgentWindowsManaged/Program.cs |
Registers the Agent event source. |
package/AgentWindowsManaged/Resources/Includes.cs |
Defines the policy-directory SDDL. |
Review details
- Files reviewed: 34/36 changed files
- Comments generated: 10
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Migration cleanup uses an incorrect Win32 field ABI, and the dependency-publication gate remains unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 38/40 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Production resource embedding and legacy ACL migration are defective, and registry dependency publication remains incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
devolutions-agent/build.rs:128
- Master builds and packages the Agent with the custom
productionprofile (.github/workflows/ci.yml:135-144, 1011, 1036), but this gate embeds the message catalog only whenPROFILEis literallyrelease. The shipped production binary will therefore lack the message-table resource that the installer registers asEventMessageFile, so Event Viewer cannot format these audit events. Include the production profile in this gate (or detect release-like profiles without relying on the profile name).
- Files reviewed: 38/40 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Boolean constraint validation is incorrect, and published registry dependencies remain an explicit merge gate.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
crates/now-package-broker/src/policy_store/validation.rs:699
- This rejects a rule whenever the match set contains
true, even when it also containsfalse. The evaluator treats{true, false}as matching either value (bool_in_set), so withAllowInteractive: falsethat rule can still match requests whereInteractiveis false and is not contradictory. Require the set to contain onlytruebefore reporting it as unreachable.
crates/sysevent-codes/src/lib.rs:389 - Event 8000 is emitted before
validate_connectionand the elevation/Administrators check inserver/mod.rs, so unauthenticated and unelevated requests also produce it. Describing every actor as an authenticated elevated Administrator makes the audit event's documented meaning inaccurate.
- Files reviewed: 38/40 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Two validation paths mishandle reachable rules, and the temporary contract dependency pins remain an explicit merge gate.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
crates/now-package-broker/src/policy_store/validation.rs:641
- Validation accepts non-empty
PackageNames, but the evaluator deliberately returns false for every such rule because package requests contain no display name (evaluator/matching.rs:25,255-259). This silently accepts an allow rule that can never apply, contrary to this validator's strict ineffective-match checks. Reject this criterion until the request contract exposes a value that can be matched.
This issue also appears on line 698 of the same file.
crates/now-package-broker/src/policy_store/validation.rs:698
- This also flags
[false, true]as contradictory merely because the set containstrue. The evaluator still permits requests whose actual value isfalse, and the constraint only rejects actualtruevalues, so that rule remains reachable. Require the match set to be exactly{true}before reporting this finding.
let mut check = |bool_match: &BTreeSet<bool>, allow_flag: bool, option_name: &str| {
- Files reviewed: 38/40 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Parent disposition for review
All 54 validator tests and the full broker run passed ( Note Human-tuned, LLM-assisted content. |
|
Parent disposition for the Event 8000 concern in review Event 8000 remains intentionally emitted before signature, elevation, and Administrators checks so denied write attempts are present in the audit trail. Its public contract now describes an attempted policy write by the OS-identified connected pipe client before final authorization; it no longer claims the actor is already authenticated or privileged. The authorization audit sequence is explicit and a focused regression proves both failed-authentication and unelevated paths emit The focused audit regression, message-catalog parity test, full broker run ( Note Human-tuned, LLM-assisted content. |
There was a problem hiding this comment.
🟡 Changes recommended
Handle-bound authentication, rollback cleanup, Unicode path comparison, and dependency publication issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
crates/now-package-broker/src/policy_security.rs:756
- This claims Windows case-insensitive comparison but only folds ASCII. Windows paths may contain non-ASCII characters and use ordinal Unicode case mapping, so a valid configured ancestor whose stored casing differs (for example, accented letters) is rejected as an unexpected location. Use Windows ordinal case-insensitive UTF-16 comparison, such as
CompareStringOrdinal(..., TRUE), rather thaneq_ignore_ascii_case.
crates/now-package-broker/src/policy_store/windows.rs:668 - This second Windows path comparator is also ASCII-only. As a result, a policy leaf with non-ASCII casing that Windows resolves successfully can be classified as a retargeted/unsafe file and made read-only. Reuse a Windows ordinal case-insensitive UTF-16 comparison helper here instead of
eq_ignore_ascii_case.
package/AgentWindowsManaged/Actions/CustomActions.cs:1953 - This releases the validated marker handle and then deletes whatever currently occupies
markerby path. A replacement marker created in that gap is therefore deleted without the identity/content binding promised by the migration cleanup. Open the marker withDELETEaccess and callDeleteFileByHandle(markerPath.Leaf)before leaving theusingblock, as the commit action already does.
- Files reviewed: 38/40 changed files
- Comments generated: 1
- Review effort level: Balanced
|
Review
Full validation passed: 430 broker tests (1 ignored, 1 documented host-specific filter), 29 Windows-wrapper tests, 26 Agent-shared tests, message-catalog parity, targeted warnings-denied Clippy, managed-installer Note Human-tuned, LLM-assisted content. |
Replace the temporary now-libraries git patches with the official registry releases from Devolutions/now-libraries#99. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Bind named-pipe clients to the process image section and retain verified executable and ancestor handles for Authenticode and ACL checks. Bound unauthenticated capture work and cover same-stream rewriting without claiming runtime or historical integrity. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep the verified marker handle open with delete access from parsing through deletion so in-place rewrites cannot preserve identity and escape validation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Allow the same-stream regression to observe either an untrusted owner or an untrusted write ACE, since both are valid fail-closed outcomes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Retain the token-observed target and publish through a durable handle-based tombstone transaction so concurrent external content is never overwritten and interrupted writes recover deterministically. Enforce the shared rule-priority maximum and rotate validation receipts with validator version 3. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Carry Phase 1's 404 inspection and 503 evaluation split while removing Phase 1-only assertions that management routes remain hidden after Phase 2 enables them. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep Phase 1's function-level large-error expectations and remove the now-redundant module-level expectation after Phase 2 route integration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
953d964 to
04d6e47
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Default-directory creation can follow an attacker-controlled junction before ancestor verification.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 39/42 changed files
- Comments generated: 1
- Review effort level: Balanced
Verify the complete existing ancestor chain before the runtime fallback creates the protected policy directory, preventing privileged side effects through a pre-created junction. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Create transactions lack retained-directory anchoring, and capability-only updates produce false policy-change audits.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/now-package-broker/src/policy_security.rs:861
- This paragraph describes ancestor verification, but it is attached to
final_path_from_handle, which only resolves a path. Remove the stale paragraph so the function's documentation matches its behavior.
- Files reviewed: 39/42 changed files
- Comments generated: 1
- Review effort level: Balanced
Create default policy components under retained verified parents, permit compatible shared-vendor ACLs while keeping PackageBroker strict, and use tested handle-relative rename transitions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Create default policy components under retained verified parents, support compatible shared-vendor ACLs, and use tested handle-relative file transitions. Avoid policy-change audit events for capability-only snapshot updates. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Legacy migration can delete a policy file that remains explicitly configured as the Agent’s active path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 39/42 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Client-token impersonation occurs before any pipe message is read, causing real connections to fail authentication.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 39/42 changed files
- Comments generated: 1
- Review effort level: Balanced
Capture authorization from the retained connector process token without premature pipe impersonation, and reject inherited connections after the original process exits. Preserve an explicitly configured legacy policy during installer migration using strict config parsing and pinned file identity. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adds authenticated transactional package-policy management to Devolutions Agent, including deterministic validation, optimistic concurrency, crash-safe JSON persistence, secure path and ACL enforcement, and stable old-or-new runtime snapshots.
Policy writes require the actual named-pipe client token to be elevated with the Administrators group enabled. Read, management, and validation remain available to an authenticated standard user. Structured tracing, Windows Event Log auditing, fail-closed watcher reconciliation, and split unelevated/LocalSystem end-to-end coverage make policy changes observable and recoverable.
Caller authentication binds a retained executable candidate to the process main-image section with
NtQueryInformationProcess(ProcessImageFileMapping), rejects network/provider image paths, verifies Devolutions Authenticode through the retained handle, and permits only trusted writers to tamper with the executable or its pinned, reparse-safe ancestor path. The same-stream regression proves that class 44 and a current valid signature are insufficient alone: a user-writable rewritten stream is rejected by the retained file and ancestor security policy.This authenticates approved image and file provenance at connection time, not runtime memory integrity or historical bytes. Same-integrity injection or hollowing of an approved non-PPL process is outside signed-image authentication. Such control grants no additional read, management, or validation authority beyond running signed UniGetUI, and a standard or medium-integrity user still cannot replace policy because the pipe token must independently be elevated and Administrators-enabled. SYSTEM and elevated-Administrator injection are inside the write trust boundary; stronger runtime integrity requires an appropriate WDAC policy or compatible PPL protection levels.
Current ACL verification cannot reconstruct historical write access. Trust assumes the approved binary and each ancestor were secure when created and were never untrusted-writable. Operators must reinstall or remediate after any ACL or path compromise before trusting the deployment again; installer and package verification establish secure deployment, while runtime checks fail closed on present insecurity.
Non-Create replacement retains the exact token-observed file and directory path, writes a durable secure transaction marker and replacement, moves the observed target to a tombstone by handle, and publishes the replacement by a no-replace handle-relative rename. Concurrent external content is never overwritten. Startup recovery restores the exact verified tombstone when the final leaf is absent, preserves any raced-in or published final leaf, and fails closed on malformed, ambiguous, or insecure remnants.
The default policy tree is traversed under retained no-delete-share handles from ProgramData. Existing shared
Devolutionsdirectories may retain compatible create-only sibling grants, but path-tampering grants are rejected; the dedicatedPackageBrokerdirectory always requires strict SYSTEM/Administrators-only security. Missing components receive protected security at creation, and race winners are reopened without following reparses and verified before traversal continues.The store preserves structured API failures, binds
ConfirmOverwriteto the exact observed store token, restricts committed revisions and rule priorities to1..=2,147,483,647where applicable, and migrates eligible legacy policy files transactionally. Migration commit and rollback retain verified marker handles through deletion so path replacement or in-place marker rewrites cannot delete newer content.Uses official crates.io releases
now-policy0.3.0,now-policy-api0.4.0, andnow-policy-server-template0.4.0 with no now-libraries git source. Committed documents usehttps://devolutions.net/schemas/now-policy.schema.1.0.json; editable drafts usehttps://devolutions.net/schemas/now-policy-draft.schema.1.0.json.Local validation includes Rust formatting, locked default and development broker suites, warnings-denied workspace Clippy, the full locked workspace suite, the i686 Windows broker check, Agent and policy-tester builds, the unelevated Agent policy end-to-end suite, direct Win32/native handle-relative rename tests, transaction crash/race recovery tests, and hostile directory creation tests. Exact-head CI provides the LocalSystem Repair and full end-to-end legs.
Stacked on #1937.
Shared contract: Devolutions/now-libraries#99 with the corrective follow-up in Devolutions/now-libraries#101.
Registry publication: Devolutions/now-libraries#98.